fix(button -> negative): corrects interior label background color to align with parent #3078

cdransf · 2024-09-06T19:15:41Z

Description

Apples forced-color-adjust: none to align the rendered background of the child label for negative button varient with the enclosing button's background color.

How and where has this been tested?

Verified in Assistiv Labs high contrast mode-enabled Windows VM.

Validation steps

Run Storybook locally (or reference the link for this PR).
Navigate to Assistiv Labs and start a Windows High Contrast Mode VM (if running the branch locally, you'll need to enable the Assistiv Labs tunnel to access Storybook).
Enable Windows High Contrast Mode.
Navigate to the Button component in Storybook.
Select the negative variant.
Zoom in and verify that the there is no background (the button background should be a uniform color).

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-09-06T19:15:45Z

🦋 Changeset detected

Latest commit: 4dc7f9e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/button	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-09-06T19:20:36Z

🚀 Deployed on https://pr-3078--spectrum-css.netlify.app

github-actions · 2024-09-06T19:25:27Z

File metrics

Summary

Total size: 4.11 MB^*
Total change (Δ): ⬆ 2.90 KB (0.07%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
button	83.96 KB	⬆ 0.99 KB

Details

button

File	Head	Base	Δ
index-base.css	54.01 KB	53.04 KB	⬆ 0.99 KB (1.82%)
index-theme.css	30.56 KB	30.56 KB	-
index-vars.css	83.96 KB	82.99 KB	⬆ 0.99 KB (1.16%)
index.css	83.96 KB	82.99 KB	⬆ 0.99 KB (1.16%)
themes/express.css	29.37 KB	29.37 KB	-
themes/spectrum.css	29.43 KB	29.43 KB	-

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

rise-erpelding

Do we need to ensure that this looks consistent in light mode as well? I'm seeing some introduced issues here that might need to be taken care of if so:

(In AssistivTunnel, with the High Contrast White theme):

(In Chrome, emulating forced colors and prefers-color-scheme: light):

cdransf · 2024-09-12T23:47:45Z

Do we need to ensure that this looks consistent in light mode as well? I'm seeing some introduced issues here that might need to be taken care of if so:

(In AssistivTunnel, with the High Contrast White theme):

(In Chrome, emulating forced colors and prefers-color-scheme: light):

✨ Huh — it looks like the button-content-color tokens are getting set in the button's index.css file but not defined. Setting them on .spectrum-Button looks good (and they can be updated by subsequent declarations specific to different states):

.spectrum-Button {
  --highcontrast-button-content-color-default: ButtonText;
  --highcontrast-button-content-color-hover: ButtonText;
  --highcontrast-button-content-color-focus: ButtonText;
  --highcontrast-button-content-color-down: ButtonText;
}

The styles aren't terribly distinct, but I imagine the available colors don't allow them to be.

rise-erpelding

Looks great!

jawinn · 2024-09-13T14:21:22Z

components/button/index.css

+ &.spectrum-Button--negative .spectrum-Button-label {
+ forced-color-adjust: none;
+ }


Should this be applied to.spectrum-Button-label on all the buttons, not just the negative variant? I see it's already applied above to the accent variant, and I am seeing the black text backplate on the primary variant:

Updated it here 853ddac#diff-97b5a7e747486aca74b3076354c28a00ef69dab3c91dc0806d926078198571a2R325 ✨

jawinn · 2024-09-13T14:32:27Z

components/button/index.css

+ --highcontrast-button-content-color-default: ButtonText;
+ --highcontrast-button-content-color-hover: ButtonText;
+ --highcontrast-button-content-color-focus: ButtonText;
+ --highcontrast-button-content-color-down: ButtonText;


In addition, I think it would be a good idea to be explicit about setting the matching background color too. Right now we're relying on the default.

--highcontrast-button-background-color-default: ButtonFace; --highcontrast-button-background-color-hover: ButtonFace; --highcontrast-button-background-color-down: ButtonFace; --highcontrast-button-background-color-focus: ButtonFace; --highcontrast-button-background-color-disabled: ButtonFace;

Updated here 853ddac#diff-97b5a7e747486aca74b3076354c28a00ef69dab3c91dc0806d926078198571a2R313 ✨

jawinn · 2024-09-13T14:37:30Z

components/button/index.css

@@ -306,6 +306,10 @@ a.spectrum-Button {
 /* Windows High Contrast Mode */
 @media (forced-colors: active) {


While we're cleaning up these high contrast styles, could you bring over some of the changes I made in the spectrum 2 migration here? The use of the Highlight background should be paired with HighlightText: 597d33f

Let me know if this lines up with what you're expecting 853ddac#diff-97b5a7e747486aca74b3076354c28a00ef69dab3c91dc0806d926078198571a2R335

Thanks for taking a look! 🙂

Thanks! Could you keep the &.spectrum-Button--accent.spectrum-Button--fill selector the same for now? Removing the fill class hasn't been done yet in S1, so keeping that the same should avoid needing changes on the SWC side.
Also, really minor but it looks like the indentation needs an update there.

Yep! Just reverted that.

jawinn

Those changes look good, thanks.

One last issue I am seeing is related to the border colors. I saw they were changing on hover of the outline variants in WHCM, but not the others. And the border colors should be the same as the background for the accent hover/focus/active.

I think adding something like this to the forced-colors styles should work? (not tested):

.spectrum-Button {
    --highcontrast-button-border-color-default: ButtonBorder;
    --highcontrast-button-border-color-hover: ButtonBorder;
    --highcontrast-button-border-color-focus: ButtonBorder;
    --highcontrast-button-border-color-down: ButtonBorder;

    --mod-button-animation-duration: 0s;

    &.spectrum-Button--accent.spectrum-Button--fill {
        --highcontrast-button-border-color-default: ButtonText;
        --highcontrast-button-border-color-hover: Highlight;
        --highcontrast-button-border-color-focus: Highlight;
        --highcontrast-button-border-color-down: Highlight;
    }
}

I also got rid of the transition duration here since the system colors don't seem to transition and look quite strange while changing on hover/focus.

I'm a little unclear about what the hover colors should be, while ensuring contrast, given that originally it looks like the design intention was to change the border color on hover.

jawinn · 2024-09-13T18:45:05Z

components/button/index.css

+ --highcontrast-button-content-color-down: HighlightText;
+ --highcontrast-button-content-color-focus: HighlightText;


These just need the indentation adjusted.

Testing these on WHCM and they behave as I believe we're expecting. The outline variation border no longer transitions on hover.

I'm curious about the intent as well — outside of WHCM there's a subtle darkening of the whole outlined button, but that isn't going to work in high contrast mode.

I'm going to push up a change that includes the four property definitions added to .spectrum-Button, the added mod (spacing included so we've got a distinction between the core properties and mods) and the updated .spectrum-Button--accent.spectrum-Button--fill definitions.

.changeset/little-cows-hang.md

components/button/index.css

…align with parent

…n fixes

Co-authored-by: Josh Winn <965114+jawinn@users.noreply.github.com>

cdransf requested a review from jawinn September 6, 2024 19:15

cdransf self-assigned this Sep 6, 2024

cdransf added run_vrt For use on PRs looking to kick off VRT ready-for-review labels Sep 6, 2024

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch 2 times, most recently from 3df3bde to 458c8b0 Compare September 6, 2024 20:12

cdransf requested a review from pfulton September 6, 2024 20:33

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from 458c8b0 to a67009e Compare September 10, 2024 14:48

cdransf requested a review from castastrophe September 10, 2024 15:28

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from a67009e to 4841afe Compare September 10, 2024 23:36

cdransf requested a review from marissahuysentruyt September 11, 2024 16:16

rise-erpelding self-requested a review September 12, 2024 23:07

rise-erpelding reviewed Sep 12, 2024

View reviewed changes

cdransf requested a review from rise-erpelding September 12, 2024 23:47

rise-erpelding approved these changes Sep 13, 2024

View reviewed changes

rise-erpelding mentioned this pull request Sep 13, 2024

fix(stepper): corrects invalid button color border to match field in windows high contrast mode #3103

Merged

9 tasks

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from 0b00a81 to 4157992 Compare September 13, 2024 14:38

jawinn requested changes Sep 13, 2024

View reviewed changes

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch 5 times, most recently from eac62c6 to dfa160a Compare September 13, 2024 16:33

cdransf enabled auto-merge (squash) September 13, 2024 16:44

cdransf requested a review from jawinn September 13, 2024 17:48

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from dfa160a to 32ccd99 Compare September 13, 2024 17:48

jawinn requested changes Sep 13, 2024

View reviewed changes

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from e0e3f3f to c96ae2b Compare September 13, 2024 21:02

cdransf closed this Sep 13, 2024

auto-merge was automatically disabled September 13, 2024 21:45
Pull request was closed

cdransf reopened this Sep 13, 2024

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from c96ae2b to 135d345 Compare September 16, 2024 16:09

cdransf requested a review from jawinn September 16, 2024 16:09

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from 135d345 to 280cab2 Compare September 16, 2024 20:54

jawinn requested changes Sep 16, 2024

View reviewed changes

.changeset/little-cows-hang.md Outdated Show resolved Hide resolved

components/button/index.css Show resolved Hide resolved

components/button/index.css Outdated Show resolved Hide resolved

cdransf requested a review from jawinn September 16, 2024 23:03

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from 5d4054e to ec2fb49 Compare September 17, 2024 13:28

cdransf and others added 7 commits September 17, 2024 08:14

fix(button -> negative): corrects interior label background color to …

430fe26

…align with parent

fix(button): sets default text colors

4f7826f

fix(button): sets default background colors; incorporates s2 migratio…

9456b9c

…n fixes

fix(button): adjusts outline border hover behavior in high contrast mode

352ce11

Update .changeset/little-cows-hang.md

dcc138e

Co-authored-by: Josh Winn <965114+jawinn@users.noreply.github.com>

fix(button): restore missing color properties; adjust indentation

ae97d5b

fix(button): adjust indentation

4dc7f9e

cdransf force-pushed the cdransf/button-high-contrast-label-fix branch from af1d020 to 4dc7f9e Compare September 17, 2024 15:14

jawinn approved these changes Sep 17, 2024

View reviewed changes

cdransf merged commit 06491f3 into main Sep 17, 2024
14 checks passed

cdransf deleted the cdransf/button-high-contrast-label-fix branch September 17, 2024 15:31

github-actions bot mentioned this pull request Sep 17, 2024

chore: release #3122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(button -> negative): corrects interior label background color to align with parent #3078

fix(button -> negative): corrects interior label background color to align with parent #3078

cdransf commented Sep 6, 2024 •

edited

Loading

changeset-bot bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

button

rise-erpelding left a comment

cdransf commented Sep 12, 2024

rise-erpelding left a comment

jawinn Sep 13, 2024

cdransf Sep 13, 2024

jawinn Sep 13, 2024

cdransf Sep 13, 2024

jawinn Sep 13, 2024

cdransf Sep 13, 2024

jawinn Sep 13, 2024 •

edited

Loading

cdransf Sep 13, 2024

jawinn left a comment •

edited

Loading

jawinn Sep 13, 2024

cdransf Sep 13, 2024

		@@ -306,6 +306,10 @@ a.spectrum-Button {
		/* Windows High Contrast Mode */
		@media (forced-colors: active) {

		--highcontrast-button-content-color-down: HighlightText;
		--highcontrast-button-content-color-focus: HighlightText;

fix(button -> negative): corrects interior label background color to align with parent #3078

fix(button -> negative): corrects interior label background color to align with parent #3078

Conversation

cdransf commented Sep 6, 2024 • edited Loading

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented Sep 6, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Sep 6, 2024 • edited Loading

github-actions bot commented Sep 6, 2024 • edited Loading

File metrics

Summary

button

rise-erpelding left a comment

Choose a reason for hiding this comment

cdransf commented Sep 12, 2024

rise-erpelding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawinn Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawinn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdransf commented Sep 6, 2024 •

edited

Loading

changeset-bot bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading

jawinn Sep 13, 2024 •

edited

Loading

jawinn left a comment •

edited

Loading